Skip to content

Conversation

aswild
Copy link
Contributor

@aswild aswild commented Apr 10, 2022

Running ./x.py build -h -v shows a list of available build targets,
but the short alias ./x.py b -h -v does not. Fix so that the aliases
behave the same as their spelled out counterparts.

Running `./x.py build -h -v` shows a list of available build targets,
but the short alias `./x.py b -h -v` does not. Fix so that the aliases
behave the same as their spelled out counterparts.
@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 10, 2022
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but I think we should consider consolidating these to be consistent at some point - there's a ton of these matches sprawled across src/bootstrap.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

Yeah, consolidating this into one place may be a little difficult but likely worthwhile.

@bors
Copy link
Collaborator

bors commented Apr 11, 2022

📌 Commit e4bbbac has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 11, 2022
@aswild
Copy link
Contributor Author

aswild commented Apr 11, 2022

Yeah, I was a bit surprised at the number of string based matches for subcommand rather than parsing into an enum early on.

The builder::Kind enum seems like it could work, but it's missing variants for Format, Clean, and Setup. flags::Subcommand is essentially what we're mapping on, but that enum has data associated with each variant that isn't built until after the subcommand type is known.

Here's a general idea for one way this could be refactored

enum SubcommandKind {
    Build,
    Check,
    ...
}
impl FromStr for SubcommandKind {...}
impl SubcommandKind {
    fn add_extra_opts(&self, opts: &mut Options) { /* flags.rs lines 279-338 */}
    fn add_extra_help(&self, subcommand_help: &mut String) { /* flags.rs lines 397-539 */ }
    fn build_subcommand(&self, paths: Vec<PathBuf>, matches: &Matches) -> Result<Subcommand> {
        /* flags.rs lines 550-624 */
    }
}

Pros of this: matching on an enum rather than string names of the commands, splitting up the very long Flags::parse() function so it's not over 500 lines long. Cons: large diff from moving so much code around may be undesirable, and now there's 3 different enums that have variants like Build, Check, Test, etc. which could be confusing.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 11, 2022
…imulacrum

bootstrap: show available paths help text for aliased subcommands

Running `./x.py build -h -v` shows a list of available build targets,
but the short alias `./x.py b -h -v` does not. Fix so that the aliases
behave the same as their spelled out counterparts.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2022
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#95743 (Update binary_search example to instead redirect to partition_point)
 - rust-lang#95771 (Update linker-plugin-lto.md to 1.60)
 - rust-lang#95861 (Note that CI tests Windows 10)
 - rust-lang#95875 (bootstrap: show available paths help text for aliased subcommands)
 - rust-lang#95876 (Add a note for unsatisfied `~const Drop` bounds)
 - rust-lang#95907 (address fixme for diagnostic variable name)
 - rust-lang#95917 (thin_box test: import from std, not alloc)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@jyn514
Copy link
Member

jyn514 commented Apr 11, 2022

The builder::Kind enum seems like it could work, but it's missing variants for Format, Clean, and Setup.

I think just adding the new variants will be simpler than trying to introduce a new type. Are you interested in working on that? I'm happy to mentor :)

@bors bors merged commit 69fb8f6 into rust-lang:master Apr 11, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants